tls: openssl: preserve syscall error details (5.0) - #12179
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4b494f8af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| errno = 0; | ||
| ret = SSL_read(backend_session->ssl, buf, len); | ||
| saved_errno = errno; |
There was a problem hiding this comment.
Capture the Winsock error for TLS syscalls
On Windows, socket failures are reported through WSAGetLastError() rather than errno, as reflected by the repository's Windows implementations of flb_socket_error() and flb_wsa_get_last_error(). Therefore a reset or timeout from SSL_read() leaves saved_errno at zero, causing SSL_ERROR_SYSCALL to be mislabeled as an unexpected EOF and recorded as generic ECONNRESET; the identical pattern affects SSL_write(). Capture the platform socket error immediately after both OpenSSL calls so Windows retains the actual failure details.
Useful? React with 👍 / 👎.
Summary
Backport the OpenSSL I/O error-classification fix and deterministic TLS regressions to
5.0.errnoimmediately afterSSL_read()andSSL_write()SSL_ERROR_SSLfailures instead of silently discarding the error queueclose_notifybehavior without importing master's separate lifecycle fixRoot cause
The
SSL_ERROR_SYSCALLpath readerrnoafterflb_errno()and logging operations could replace it with an unrelated value such asENOTTY. The read path also used a deadssl_ret < 0condition;SSL_get_error()returns nonnegative constants, soSSL_ERROR_SSLand other fatal failures were silently discarded and leftconnection->net_error == -1.The backport classifies failures as follows:
SSL_ERROR_SYSCALLwith saved nonzeroerrno: report and preserve the real syscall failureSSL_ERROR_SYSCALLwith an empty queue and zero errno: report legacy unexpected EOF and useECONNRESETas the terminal markerSSL_ERROR_ZERO_RETURN: return terminally without logging, preserving the 5.0 baseline behaviorWANT_READ/WANT_WRITE behavior is unchanged. This branch does not import master's #12079 close-notify recycling fix, #12141's retry work, or any handshake EOF behavior change.
Compatibility and severity
An established-session peer EOF without
close_notifyis logged at error level. OpenSSL 3.x classifies this as a fatal TLS truncation/protocol error, and older OpenSSL versions already reached the error-levelSSL_ERROR_SYSCALLpath. This may make abrupt disconnects from non-compliant clients more visible, but Fluent Bit does not enableSSL_OP_IGNORE_UNEXPECTED_EOFbecause that would discard TLS truncation protection. An abrupt EOF during an incompleteSSL_write()is also error-level because application data may have been lost. A cleanclose_notifyremains silent on 5.0.Regression proof
The known-broken syscall path emitted the real reset followed by an unrelated
Inappropriate ioctl for device; it now reports onlyConnection reset by peer. A corrupted encrypted record previously reachedSSL_ERROR_SSLwithout a TLS log; it now reports the authoritativebad record macqueue entry.The initial backport catch-all also reported a clean shutdown as:
An explicit
SSL_ERROR_ZERO_RETURNbranch removes that regression while preserving the 5.0 baseline. All three tests submit a valid TLS Forward payload afterward and verify responsiveness.Validation
All three normal and strict-Valgrind tests pass. Valgrind reports 0 errors, all heap blocks freed, and no allocations remaining at exit. The full six-commit range passes the repository prefix checker against
5.0.